JavaScript: Add queries and libraries for supporting flow summaries by xiemaisi · Pull Request #502 · github/codeql

xiemaisi · 2018-11-20T15:45:05Z

This PR introduces queries and libraries that can be used to generate and consume flow summaries for npm packages using the command-line tools and QL4E.

It's still quite experimental and unfinished, but I'm opening this PR anyway to serve as a basis for discussion.

See flow-summaries.rst included in this PR for a brief overview of the various bits of machinery and how to use them.

asger-semmle

I've read the .rst file and tried following its instructions (very nicely written!), but couldn't get it working, possibly because of how I randomly cut off the negative recursion with a blunt axe.

I'll give it a spin next week, and review it once I have a better feeling for how it works.

asger-semmle · 2018-11-30T19:01:13Z

+  or
+  spec = "" and
+  not config.isBarrier(_) and
+  not config.isBarrier(_, _)


There seems to be negative recursion going through here.

The characteristic predicate of the dataflow configuration in ImportFromCsv depends on configSpec which depends on the dispatch predicate of isBarrier, which depends on the set of dataflow configurations.

Oh dear, I wonder when that crept in. I'll take a look on Monday.

Fixed by removing the two negative conjuncts. They weren't a good idea in the first place.

asger-semmle

A few comments so far:

The two ways to import summaries aren't fundamentally mutually exclusive are they? It seems we could safely add ImportFromCsv in javascript.qll and still have ImportFromExternalPredicates as an optional throw-in when using Eclipse (after a bit of code deduplication to avoid name clashes).

asger-semmle · 2018-12-03T14:04:28Z

+  }
+}
+
+private class AdditionalFlowStepFromSpec extends DataFlow::Configuration {


It would be nice with a dedicated AdditionalFlowStep class for this instead, like with the sources and sinks.

Sort of related: The flow summaries only seem to track TaintTracking::Configuration. Is that deliberate? Is there a reason we can't use AdditionalTaintStep?

Hm, good point. There isn't a fundamental reason to only track TaintTracking::Configurations, but it's probably the case we are most interested in for now, so I'll just switch this to AdditionalTaintStep.

Ugh, except that AdditionalTaintStep doesn't currently allow specifying kinds, and it is also cached and should only be extended in the standard library. Bit messy, that.

Ah, yeah the cached thing is a deal-breaker. Still, it seems a bit sneaky to have this configuration-class here, so if we could add an uncached class for this that would be great.

asger-semmle · 2018-12-03T14:15:21Z

+      p1 = source.getNode().(PortalExitSource).getPortal() and
+      p2 = sink.getNode().(PortalEntrySink).getPortal() and
+      lbl1 = sink.getPathSummary().getStartLabel() and
+      lbl2 = sink.getPathSummary().getStartLabel() and


Copy-pasta? The two labels will always be the same, won't they?

Indeed; my least favourite kind of pasta.

xiemaisi · 2018-12-03T15:02:51Z

It seems we could safely add ImportFromCsv in javascript.qll

Yes, I think that would be a good idea, but I'll need to check that it doesn't unduly affect performance when there aren't any summaries to import.

… labels.

If a summary does not specify a configuration, it is taken to apply to all configurations without custom sanitisers/barriers. If a source summary does not specify a flow label, `data` is assumed. If a sink summary does not specify a flow label, both `data` and `taint` are assumed. Flow step summaries cannot omit flow labels. Note that the standard extraction queries always provide explicit configurations and flow labels, and hence do not exercise this functionality.

… sources and sinks.

In its current form, this query produces way too many results.

…f many configurations with many sources/sinks.

…bility.

This reverts commit d03f82b.

xiemaisi · 2019-01-09T09:18:18Z

Rebased to resolve auto-formatting conflicts.

I've experimented with ways of reducing the performance penalty from including ImportFromCSV in the standard library, but couldn't bring the overhead down to acceptable levels, so I've reverted that commit. With the revert, no performance or result changes were observed on a full dist-compare run.

This means that flow-summary support is opt-in and requires a local change to javascript.qll, but perhaps that's good enough for now.

asger-semmle

LGTM then

github#698 removed `document.cookie` as a remote flow source, which some of the tests relied on. We now use `location.search` instead.

xiemaisi · 2019-01-11T07:33:22Z

@asger-semmle: I had to add a commit to resolve a semantic merge conflict (document.cookie, which was used in one of the tests) is no longer considered a source of remote flow. Could you take a look?

asger-semmle

Still LGTM

…ally Find latest release of the CLI automatically

xiemaisi added JS WIP This is a work-in-progress, do not merge yet! labels Nov 20, 2018

xiemaisi requested a review from a team as a code owner November 20, 2018 15:45

xiemaisi force-pushed the js/summaries branch from 1e35d33 to 6eeb533 Compare November 22, 2018 22:00

asger-semmle reviewed Nov 30, 2018

View reviewed changes

xiemaisi force-pushed the js/summaries branch from 6eeb533 to 4791db2 Compare December 3, 2018 09:34

asger-semmle reviewed Dec 3, 2018

View reviewed changes

xiemaisi force-pushed the js/summaries branch 2 times, most recently from da357e3 to 6644597 Compare December 13, 2018 10:51

xiemaisi force-pushed the js/summaries branch 5 times, most recently from ab2cfc6 to 1eb61f3 Compare January 4, 2019 08:26

Max Schaefer added 15 commits January 9, 2019 09:05

JavaScript: Add predicate AbstractCallable.getDefinition.

3e56e9e

JavaScript: Add predicate DataFlow::Node.getTopLevel().

7e7899f

JavaScript: Add QL library for modelling portals between npm packages.

98a763a

JavaScript: Allow additional sources, sinks and steps to specify flow…

6d893d4

… labels.

JavaScript: Add flow summary extraction queries.

f4fed36

JavaScript: Import flow summaries from CSV data.

90ad8e3

JavaScript: Import flow summaries through external predicates.

7c87c43

JavaScript: Add support for annotation comments specifying additional…

1325709

… sources and sinks.

JavaScript: Add a few examples that cause cyclic portals.

8e36c60

JavaScript: Add guide to using summaries.

fae419c

JavaScript: Exclude step summary query from flow-summaries suite.

94242b3

In its current form, this query produces way too many results.

JavaScript: Add ImportFromCSV to javascript.qll.

2295353

JavaScript: Cache portal computation.

9a64224

JavaScript: Fold a predicate to improve performance in the presence o…

a7ea730

…f many configurations with many sources/sinks.

Max Schaefer added 3 commits January 9, 2019 09:10

JavaScript: Change encoding of member and parameter portals for reada…

8f1c5db

…bility.

JavaScript: Make configuration IDs explicit in the API.

e960bd9

Revert "JavaScript: Add ImportFromCSV to javascript.qll."

fb53a69

This reverts commit d03f82b.

xiemaisi force-pushed the js/summaries branch from 1eb61f3 to fb53a69 Compare January 9, 2019 09:10

JavaScript: Autoformat new libraries.

3d44f0c

xiemaisi removed the WIP This is a work-in-progress, do not merge yet! label Jan 9, 2019

asger-semmle previously approved these changes Jan 9, 2019

View reviewed changes

JavaScript: Fix semantic merge conflict.

583734a

github#698 removed `document.cookie` as a remote flow source, which some of the tests relied on. We now use `location.search` instead.

xiemaisi dismissed asger-semmle’s stale review via 583734a January 9, 2019 16:09

asger-semmle approved these changes Jan 11, 2019

View reviewed changes

semmle-qlci merged commit b0dd3df into github:master Jan 11, 2019

xiemaisi deleted the js/summaries branch January 17, 2019 09:33

cklin pushed a commit that referenced this pull request May 23, 2022

Merge pull request #502 from owen-mc/find-latest-codeql-cli-automatic…

8318dcf

…ally Find latest release of the CLI automatically

Uh oh!

Conversation

xiemaisi commented Nov 20, 2018

Uh oh!

asger-semmle left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asger-semmle left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xiemaisi commented Dec 3, 2018

Uh oh!

xiemaisi commented Jan 9, 2019

Uh oh!

asger-semmle left a comment

Choose a reason for hiding this comment

Uh oh!

xiemaisi commented Jan 11, 2019

Uh oh!

asger-semmle left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants